Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

과제 #37

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

과제 #37

wants to merge 2 commits into from

Conversation

96uoow
Copy link

@96uoow 96uoow commented Apr 5, 2023

쏘카 페이지를 클론 코딩했습니다.

원본 페이지 : https://www.socar.kr/
과제 페이지 : https://genuine-palmier-371aa9.netlify.app/

Copy link

@sangheon-kim sangheon-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적으로 고생 많으셨습니다

아쉬운점은 코멘트 남겨드렸습니다

css파일 분기는 이해가 되지않네요 기준점을 잡아두고 미디어쿼리를 쓰셨으면 좋았을텐데요

<link rel="stylesheet" type="text/css" href="//db.onlinewebfonts.com/c/79e4dc6a81171b1523795cb6b2bc7b2f?family=Avenir+LT+Pro+55+Roman"/>
<link rel="stylesheet" href="./css/common.css">
<link rel="stylesheet" href="./css/mobile.css">
<link rel="stylesheet" media="all and (min-width:770px)" href="./css/pc.css">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미디어쿼리를 쓰지 않으신 이유가 있나요?

<meta property="og:url" content="https://www.socar.kr/">

<link rel="icon" href="https://www.socar.kr/images/i-appicon.svg">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/reset.min.css">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안에 파일은 확인해보시고 사용하시는거맞나요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보통 reset.css를 안에 내용도 확인안하고 사용해서 css를 overwrite이슈가 생기는 케이스를 여럿 봤어서 말씀드렸습니다

<section class="main-visual scroll-spy">
<header>
<a href="/"><img src="./images/logo-w.svg" alt="logo" id="logo"></a>
<div class="menubar">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nav태그를 쓰지 않으신 이유가 있나요?


<!-- DOWNLOAD BANNER -->
<div class="download">
<a href="javascript:void(0)">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

href에 그냥 #같은걸로 넣어주는게 훨씬 보기 좋을 것 같아요


const spyEls = document.querySelectorAll('section.scroll-spy');
spyEls.forEach(function(spyEl){
new ScrollMagic

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 쓰는게 맞는걸까요??

만약 이 기능이 해당 지점에 보이는걸 구현한거였다면, Intersection Observer를 알아보셔도 좋을 것 같아요

this.timer = null;
this.counter();
};
numberCounter.prototype.counter = function() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prototype 메서드를 쓰는 시도는 잘했지만,

numberCounter를 한번만 쓸거라면 차라리 클로저를 사용해보거나, 클래스를 사용해보는 방법도 생각해볼 수 있을 것 같아요


// members 영역 숫자 올라가는 효과

function numberCounter(target_frame, target_number) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 기본적으로 생성자 함수로 사용하실거라면 시맨틱상 맨 앞글자는 대문자가 원칙입니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants